Skip to content

Add a new mismatched-lifetime-syntaxes lint #138677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shepmaster
Copy link
Member

@shepmaster shepmaster commented Mar 18, 2025

The lang-team discussed this and I attempted to summarize their decision. The summary-of-the-summary is:

  • Using two different kinds of syntax for elided lifetimes is confusing. In rare cases, it may even lead to unsound code! Some examples:

    // Lint will warn about these
    fn(v: ContainsLifetime) -> ContainsLifetime<'_>;
    fn(&'static u8) -> &u8;
  • Matching up references with no lifetime syntax, references with anonymous lifetime syntax, and paths with anonymous lifetime syntax is an exception to the simplest possible rule:

    // Lint will not warn about these
    fn(&u8) -> &'_ u8;
    fn(&'_ u8) -> &u8;
    fn(&u8) -> ContainsLifetime<'_>;
  • Having a lint for consistent syntax of elided lifetimes will make the future goal of warning-by-default for paths participating in elision much simpler.


This new lint attempts to accomplish the goal of enforcing consistent syntax. In the process, it supersedes and replaces the existing elided-named-lifetimes lint, which means it starts out life as warn-by-default.

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 18, 2025
@rust-log-analyzer

This comment has been minimized.

@shepmaster
Copy link
Member Author

Thanks to:

  • @GrigorenkoPV for the original implementation of elided_named_lifetimes.
  • @compiler-errors for the push to write the lint using HIR instead of in rustc_resolve.
  • @estebank for the diagnostic help and wording feedback.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 478a98e to e15c083 Compare March 18, 2025 21:01
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from e15c083 to ca86221 Compare March 20, 2025 13:34
@shepmaster shepmaster changed the title Add a new mismatched_elided_lifetime_styles lint Add a new mismatched-lifetime-syntaxes lint Mar 20, 2025
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from ca86221 to 6b4ba02 Compare March 20, 2025 13:57
@rust-log-analyzer

This comment has been minimized.

@traviscross traviscross dismissed their stale review March 20, 2025 19:33

Addressed.

@bors
Copy link
Collaborator

bors commented Mar 22, 2025

☔ The latest upstream changes (presumably #138719) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 6b4ba02 to 7ff70d4 Compare March 23, 2025 00:48
@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Mar 23, 2025
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 7ff70d4 to 3a95f66 Compare March 24, 2025 17:29
@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 3a95f66 to 1625496 Compare March 24, 2025 20:01
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 1625496 to 1cd8999 Compare March 25, 2025 00:55
@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 573f332 to 702e59d Compare April 1, 2025 13:39
@bors
Copy link
Collaborator

bors commented Apr 4, 2025

☔ The latest upstream changes (presumably #139336) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch 2 times, most recently from e5616d0 to e6dafb6 Compare April 7, 2025 16:44
@bors
Copy link
Collaborator

bors commented Apr 15, 2025

☔ The latest upstream changes (presumably #139826) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from e6dafb6 to 5611e54 Compare April 15, 2025 12:34
@bors
Copy link
Collaborator

bors commented Apr 17, 2025

☔ The latest upstream changes (presumably #139938) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 5611e54 to 8d56830 Compare April 24, 2025 13:44
@shepmaster
Copy link
Member Author

r? compiler

@rustbot rustbot assigned BoxyUwU and unassigned wesleywiser Apr 24, 2025
@bors
Copy link
Collaborator

bors commented May 2, 2025

☔ The latest upstream changes (presumably #140581) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rustbot author


struct S(u8);

fn named_ref_to_hidden_ref<'a>(v: &'a u8) -> &u8 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is named to elided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree that this test name should be changed.

These test names are based on the syntax used, not the behavior. If they were not, there would be no naming scheme to disambiguate these materially different cases:

fn named_ref_to_elided_ref<'a>(v: &'a u8) -> &u8
fn named_ref_to_elided_ref<'a>(v: &'a u8) -> &'_ u8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That enum was added recently by you as part of this same work. I'd probably recommend changing that as well. Otherwise it's going to be one of those continually confusing areas where the compiler and the lang team / Reference disagree about terminology.

Copy link
Contributor

@traviscross traviscross May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lang team reviews UI tests to evaluate stabilizations and for other such purposes. If we want to actually use the terms differently between how we've decided to use them on the lang side and how we use them in the compiler, I feel like we're going to need to write this down carefully somewhere and then cite it widely in all the places we divergently use these terms.

(I'd prefer we find a way to avoid that and to either use the same terms to mean the same things or, if we can't align our concepts, at least to use non-overlapping terms.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That enum was added recently by you as part of this same work

Yes. The only reason it's not part of this PR is that it was blocking other compiler contributors and was extracted to a separate PR. I link to it as a listing of the choices available, not to prove its authoritativeness.

one of those continually confusing areas where the compiler and the lang team / Reference disagree about terminology

I see the point and wish there were not confusion, but there are two related yet different concepts:

  • What the user typed / what the source code shows (what I'm calling "syntax")
  • The higher-level meaning (what I'm calling "style")

I assert that using the same name for the syntax of &T and &'_ T will likely be confusing to users 1 and will almost certainly be confusing to compiler developers. That's why I've chosen the implementation path of having the two enums (LifetimeSyntax, LifetimeSource) as the primary representation through the compiler and restricted the confusing meaning to the lint.

User-facing parts of the lint (the documentation, lint messages) should indeed use the lang-team-decided terminology of named/elided/hidden where needed.

For my own part (which I don't think I've done a good job expressing), I think that saying that fn x(&u8) -> ContainsLifetime has one "elided lifetime" and one "hidden lifetime" when the action of lifetime elision occurs equally for both will already confuse users 2.

I'd prefer we find a way to avoid that and to either use the same terms to mean the same things or, if we can't align our concepts, at least to use non-overlapping terms.

That would be wonderful, but I'm not seeing how to align the terms completely because of the intrinsic differences. The source code allows for the product of {Reference, Path} ✕ {NoSyntax, AnonymousLifetime, NamedLifetime} but meaning-wise we group it as

  • (Reference, NoSyntax) / (Reference, AnonymousLifetime) / (Path, AnonymousLifetime) ("elided")
  • (Path, NoSyntax) ("hidden")
  • (Reference, NamedLifetime) / (Path, NamedLifetime) ("named")

At least until the HIR level, I argue that the compiler should correspond to what the user entered.

Footnotes

  1. e.g. "oh, change this to use an 'elided lifetime'"... "what kind?" ... "i dunno what to call it"

  2. e.g. "so since this one is hidden and this one is elided, that means that lifetime elision doesn't occur here?" ... "well, no, elision does occur, it's just not called elided"

Copy link
Contributor

@traviscross traviscross May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the concern is syntactic, and you want to apply the same categorization to both references and paths, I'd suggest that either can have an "explicit" lifetime parameter (e.g. &'a (), &'_ (), Ref<'a>, Ref<'_>) or an "implicit" one (e.g. &(), Ref), and that explicit lifetime parameters can be either a "bound" lifetime (e.g. &'a (), Ref<'a>), the "anonymous" one (e.g. &'_ (), Ref<'_>), or 'static.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adopted these terms from the enum, through the lint implementation, and out to the test.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Copy link
Member Author

@shepmaster shepmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to address the corrections I agree with, but wanted to comment quickly in an attempt to reduce the month-long review cycle by a few hours.


struct S(u8);

fn named_ref_to_hidden_ref<'a>(v: &'a u8) -> &u8 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree that this test name should be changed.

These test names are based on the syntax used, not the behavior. If they were not, there would be no naming scheme to disambiguate these materially different cases:

fn named_ref_to_elided_ref<'a>(v: &'a u8) -> &u8
fn named_ref_to_elided_ref<'a>(v: &'a u8) -> &'_ u8

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 9da1ebb to 9c8a30a Compare May 9, 2025 15:34
@shepmaster
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2025
@rust-log-analyzer

This comment has been minimized.

@shepmaster shepmaster force-pushed the consistent-elided-lifetime-syntax branch from 9c8a30a to 4581524 Compare May 9, 2025 20:02
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
tests/pass/float_nan.rs ... ok
tests/pass/0weak_memory_consistency_sc.rs ... ok

FAILED TEST: tests/pass/fat_ptr.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-JPw1yW" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/miri_ui/0/tests/pass" "tests/pass/fat_ptr.rs" "--edition" "2021"

error: test got exit status: 1, but expected 0
 = note: compilation failed, but was expected to succeed

error: no output was expected
Execute `./miri test --bless` to update `tests/pass/fat_ptr.stderr` to the actual output
+++ <stderr output>
error: lifetime flowing from input to output with different syntax
##[error]  --> tests/pass/fat_ptr.rs:22:27
   |
LL | fn fat_ptr_from_struct(s: FatPtrContainer) -> &[u8] {
   |                           ^^^^^^^^^^^^^^^     ----- the elided lifetime gets resolved as `'_`
   |                           |
   |                           this lifetime flows to the output
   |
   = note: `-D mismatched-lifetime-syntaxes` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(mismatched_lifetime_syntaxes)]`
help: one option is to consistently use `'_`
   |
LL | fn fat_ptr_from_struct(s: FatPtrContainer<'_>) -> &'_ [u8] {
   |                                          ++++      ++

error: lifetime flowing from input to output with different syntax
##[error]  --> tests/pass/fat_ptr.rs:26:25
   |
LL | fn fat_ptr_to_struct(a: &[u8]) -> FatPtrContainer {
   |                         ^^^^^     --------------- the elided lifetime gets resolved as `'_`
   |                         |
   |                         this lifetime flows to the output
   |
help: one option is to consistently use `'_`
   |
LL | fn fat_ptr_to_struct(a: &'_ [u8]) -> FatPtrContainer<'_> {
   |                          ++                         ++++

error: miri cannot be run on programs that fail compilation

error: aborting due to 3 previous errors



full stderr:
error: lifetime flowing from input to output with different syntax
##[error]  --> tests/pass/fat_ptr.rs:22:27
   |
LL | fn fat_ptr_from_struct(s: FatPtrContainer) -> &[u8] {
   |                           ^^^^^^^^^^^^^^^     ----- the elided lifetime gets resolved as `'_`
   |                           |
   |                           this lifetime flows to the output
   |
   = note: `-D mismatched-lifetime-syntaxes` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(mismatched_lifetime_syntaxes)]`
help: one option is to consistently use `'_`
   |
LL | fn fat_ptr_from_struct(s: FatPtrContainer<'_>) -> &'_ [u8] {
   |                                          ++++      ++

error: lifetime flowing from input to output with different syntax
##[error]  --> tests/pass/fat_ptr.rs:26:25
   |
LL | fn fat_ptr_to_struct(a: &[u8]) -> FatPtrContainer {
   |                         ^^^^^     --------------- the elided lifetime gets resolved as `'_`
   |                         |
   |                         this lifetime flows to the output
   |
help: one option is to consistently use `'_`
   |
LL | fn fat_ptr_to_struct(a: &'_ [u8]) -> FatPtrContainer<'_> {
   |                          ++                         ++++

error: miri cannot be run on programs that fail compilation

error: aborting due to 3 previous errors

---

Location:
   /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.29.2/src/lib.rs:369

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-d45e9ad25b356c91` (exit status: 1)
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:03:50
  local time: Fri May  9 20:35:17 UTC 2025
  network time: Fri, 09 May 2025 20:35:18 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants